fix: Invalid Date Display in Table Widget's Date Column When Using Unix Timestamp (ms) #36455
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…o fix/incorrect-selected-date-in-table-date-cell
jacquesikot
added
ok-to-test
jacquesikot
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
WalkthroughThe pull request introduces enhancements to the date column editing functionality in a table widget. It adds a test case that verifies the acceptance of Unix timestamps in milliseconds, ensuring they do not trigger errors during inline editing. Additionally, modifications are made to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
Bug
sagar-qa007
requested changes
Sep 26, 2024
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/fixtures.ts
Outdated
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/helpers.ts
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/cypress/fixtures/tableDateColumnTypes.ts (1)
2-26: Attention, students! Let's review the content of our date formats.While the variety of date formats is impressive, we need to address some inconsistencies:
Property naming conventions:
- Most properties use quotes, but
iso8601doesn't. Let's be consistent.- Some use camelCase (
unixms), others use snake_case (yyyymmdd). Choose one style.Time representation:
- Some formats use 24-hour time (
14:30), others use 12-hour time (2:30 PM). Consider standardizing.Redundancy:
llanddmmmmyyyyrepresent the same format. Consider removing one.Here's an example of how to improve consistency:
{{ [ { "unixs": 1727212200, "unixms": 1727212200000, "yyyymmdd": "2024-09-25", "yyyymmddhhmm": "2024-09-25 14:30", - iso8601: "2024-09-25T14:30:00.000Z", + "iso8601": "2024-09-25T14:30:00.000Z", "yyyymmddTHHmmss": "2024-09-25T14:30:00", "yyyymmddhhmmss": "2024-09-25 14:30:00", "doMMMyyyy": "25th Sep 2024", "ddmmyyyy": "25/09/2024", "ddmmyyyyhhmm": "25/09/2024 14:30", "lll": "September 25, 2024 2:30 PM", - ll: "September 25, 2024", + "ll": "September 25, 2024", "dmmmmyyyy": "25 September, 2024", "hmmAdmmmmyyyy": "2:30 PM 25 September, 2024", "mm1dd1yyyy": "09-25-2024", "dd1mm1yyyy": "25-09-2024", "ddimmiyy": "25/09/24", "mmddyy": "09/25/24", }, ] }}Remember, consistency is key in programming, just like in your homework!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/Date_column_types_validation_spec.ts (1 hunks)
- app/client/cypress/fixtures/tableDateColumnTypes.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/Date_column_types_validation_spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/fixtures/tableDateColumnTypes.ts (1)
Pattern
app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
🔇 Additional comments (1)
app/client/cypress/fixtures/tableDateColumnTypes.ts (1)
1-1: Well done, class! The constant declaration looks correct.The naming convention and export statement are appropriate for this TypeScript file. Using a template string for the multi-line content is a good choice.
rahulbarwal
previously approved these changes
Sep 27, 2024
rahulbarwal
previously approved these changes
Sep 27, 2024
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/Date_column_types_validation_spec.ts (1 hunks)
- app/client/cypress/support/Pages/Table.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/Date_column_types_validation_spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/support/Pages/Table.ts (1)
Pattern
app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
Comment on lines
+859
to
+884
| public getFormattedTomorrowDates() { | ||
| // Create a new Date object for today | ||
| const tomorrow = new Date(); | ||
|
|
||
| // Set the date to tomorrow by adding 1 to today's date | ||
| tomorrow.setDate(tomorrow.getDate() + 1); | ||
|
|
||
| // Format tomorrow's date in verbose form (e.g., "Sat Sep 21 2024") | ||
| const verboseFormat = tomorrow | ||
| .toLocaleDateString("en-US", { | ||
| weekday: "short", | ||
| year: "numeric", | ||
| month: "short", | ||
| day: "2-digit", | ||
| }) | ||
| .replace(/,/g, ""); // Remove commas from the formatted string | ||
|
|
||
| // Format tomorrow's date in ISO form (e.g., "2024-09-21") | ||
| const isoFormat = tomorrow.toISOString().split("T")[0]; // Extract the date part only | ||
|
|
||
| // Return both formatted date strings as an object | ||
| return { | ||
| verboseFormat, | ||
| isoFormat, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Class, let's improve our new method!
The getFormattedTomorrowDates method is a fine addition to our Table class. However, we can make it even better with a few tweaks:
-
Let's rename it to
getFormattedTomorrowDateStringsto be more specific about returning multiple formats. -
We should use JSDoc comments for better documentation. Here's an example:
/**
* Gets formatted date strings for tomorrow's date.
* @returns {Object} An object containing formatted date strings.
* @property {string} verboseFormat - Date in verbose format (e.g., "Sat Sep 21 2024").
* @property {string} isoFormat - Date in ISO format (e.g., "2024-09-21").
*/- Let's add TypeScript type annotations for better type safety:
public getFormattedTomorrowDateStrings(): { verboseFormat: string; isoFormat: string } {
// ... method implementation ...
}- Consider using
padStartfor consistent day formatting:
day: "2-digit" -> day: (tomorrow.getDate().toString().padStart(2, '0'))Now, who would like to come to the board and implement these improvements?
sagar-qa007
previously approved these changes
Sep 27, 2024
jacquesikot
added
ok-to-test
sagar-qa007
approved these changes
Sep 30, 2024
jacquesikot
deleted the
fix/incorrect-selected-date-in-table-date-cell
branch
This was referenced Sep 30, 2024
1 task
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Problem
When populating a table widget with data that includes dates in Unix timestamp (milliseconds) format, setting the column type to "Date," the input format to "Unix timestamp (ms)," and selecting a display format leads to an issue during inline editing. While the date picker behaves correctly, selecting a new date results in the table cell showing an "Invalid Date" error.
Root Cause
The platform currently uses DateInputFormat.MILLISECONDS for Unix timestamp (ms) formatting. However, this value is not a valid option for the moment.format() function, which expects the input format to be 'x' for Unix timestamps in milliseconds. This mismatch leads to the "Invalid Date" error.
Solution
Modify the logic to map DateInputFormat.MILLISECONDS to the correct moment format string 'x'.
Adjust the table's transformDataPureFn to correctly process and display dates in Unix timestamp (ms) format after inline edits, ensuring the moment library can handle the input properly.
Outcome
This fix ensures that when a user selects a date via the date picker in inline editing mode, the selected date is displayed correctly in the table without any errors.
Fixes #35631
Automation
/ok-to-test tags="@tag.Sanity, @tag.Binding, @tag.Table, @tag.Datepicker"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11101758400
Commit: 6a3cae7
Cypress dashboard.
Tags:
@tag.Sanity, @tag.Binding, @tag.Table, @tag.DatepickerSpec:
Mon, 30 Sep 2024 08:54:58 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests